Skip to content

Conversation

@ajs6f
Copy link
Member

@ajs6f ajs6f commented Mar 21, 2018

Adds configuration for use with Trellis-Cassandra.

@acoburn
Copy link
Member

acoburn commented Mar 21, 2018

This seems fine, but I wonder if it wouldn't be better for the trellis-over-cassandra-app to subclass TrellisConfiguration and add these methods there?

@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

You tell me-- I know as much about Dropwizard as I do about playing poker: nothing. Should I close this and try that instead?

@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

Nope, Java's bottom-of-the-barrel variance. TrellisApplication extends Application<TrellisConfiguration>, and TrellisCassandraApplication extends TrellisApplication, so I don't think I can also somehow TrellisCassandraApplication extends Application<TrellisCassandraConfiguration>. Or maybe I'm missing something?

@ajs6f ajs6f closed this Mar 21, 2018
@ajs6f ajs6f reopened this Mar 21, 2018
@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

Sorry-- misclick.

@acoburn
Copy link
Member

acoburn commented Mar 21, 2018

A better approach might be to add a @JsonAnySetter method to TrellisConfiguration:

@JsonAnySetter
public void set(final String name, final String value) {
    internalMap.put(name, value);
}

@JsonAnyGetter
public Map<String, String> any() {
    return internalMap;
}

That adds a simple extension point for any possible subclasses.

@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

Where is internalMap? Is that to be added as well?

@acoburn
Copy link
Member

acoburn commented Mar 21, 2018

Yes, it's just some internal field. Call it whatever you'd like.

@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

I'm not much with JSON; what about Map<String, Object>? Would that effectively account for nesting? I.e. with

@JsonAnySetter
public void set(final String name, final Object value) {
    internalMap.put(name, value);
}

@JsonAnyGetter
public Map<String, Object> any() {
    return internalMap;
}

@acoburn
Copy link
Member

acoburn commented Mar 21, 2018

I haven't ever used that precise construct with Jackson serializations, but I don't see why it wouldn't work. It should be trivial to set up a test -- just add some additional fields to one of the configuration files in ./src/test/resources

@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

Didn't work at all, not even with simple Strings:

io.dropwizard.configuration.ConfigurationParsingException: /Users/ajs6f/Documents/trellis/trellis/trellis-app/bin/test/config1.yml has an error:
  * Unrecognized field at: cassandraAddress
    Did you mean?:
      - assets
      - resources
      - namespaces
      - cacheMaxAge
      - binaries
        [12 more]

So maybe that works for Jackson's semantics, but not for Dropwizard's particular concrete processing code?

@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

Again, I know little about Dropwizard, but I'm beginning to suspect that we're running up against its intentional limits-- it seems to be for building single apps, not ecosystems. Extensible config is really not a concern if you are build a single app.

@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

Hang on-- I had a misconfig in my code. Testing further...

@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

Okay, how's that?

@ajs6f ajs6f changed the title Offering Cassandra cluster metadata from configuration Offering extended configuration Mar 21, 2018
@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

incidentally, I tried going:

public class TrellisApplication extends Application<TrellisConfiguration> {

=>

public class TrellisApplication<T extends TrellisConfiguration> extends Application<T> {

but all of the test classes exploded with:

The constructor DropwizardTestSupport<TrellisConfiguration>(Class<TrellisApplication>, String, ConfigOverride, ConfigOverride, ConfigOverride) is undefined

Not really sure what's going on there, but I think the issue is that Class<TrellisApplication> isn't provably a supertype of Class<TrellisApplication<T extends TrellisConfiguration>> (or the other way 'round), so the compiler pukes into its own shirt. Shall we just rewrite the whole app in Scala?

@christopher-johnson
Copy link
Member

@ajs6f I tried running the test and it works for me using IntelliJ. One side note is that the @RunWith(JUnitPlatform.class) is not required since the test class is written for JUnit5. AFAIK, that annotation invokes the JUnit4 runner.

@ajs6f
Copy link
Member Author

ajs6f commented Mar 21, 2018

Thanks much @christopher-johnson ! I didn't actually write that test, just added a few limes to it, so I'll let @acoburn remove that annotation. Unless, @acoburn , you'd like me to do it here?

Copy link
Member

@acoburn acoburn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. There are some trivial checkstyle-related issues to be fixed before merging


import static java.util.Collections.synchronizedMap;

import java.util.Collections;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used and can be removed.


import io.dropwizard.Configuration;

import static java.util.Collections.synchronizedMap;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move static imports to the top (before ordinary imports)

private String baseUrl = null;

private String resourceLocation = null;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace

assertEquals("my.cluster.node", config.any().get("cassandraAddress"));
assertEquals((Integer)245993, config.any().get("cassandraPort"));
@SuppressWarnings("unchecked")
Map<String, Object> extraConfig = (Map<String, Object>) config.any().get("extraConfigValues");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add final here.

Map<String, Object> extraConfig = (Map<String, Object>) config.any().get("extraConfigValues");
assertTrue((Boolean) extraConfig.get("one"));
@SuppressWarnings("unchecked")
List<String> list = (List<String>) extraConfig.get("two");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add final

@ajs6f
Copy link
Member Author

ajs6f commented Mar 22, 2018

I will fix those probs in two seconds, but just so you know-- I ran ./gradlew check install to check this and it passed fine. Do we need to couple checkstyleMain into check? I thought it already was?

@acoburn
Copy link
Member

acoburn commented Mar 22, 2018

That's odd. It could be that running this without clean means that the changes aren't caught. I definitely get the checkstyle errors when running ./gradlew clean check install

@acoburn acoburn merged commit 050d873 into trellis-ldp:master Mar 22, 2018
@ajs6f
Copy link
Member Author

ajs6f commented Mar 22, 2018

Okay, I'll keep an eye peeled. I might try making a few intentional faux pas and see which Gradle invocations catch them and which do not.

acoburn pushed a commit that referenced this pull request Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants